-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: hook xblock publish, delete and duplicate openedx-events #31350
feat: hook xblock publish, delete and duplicate openedx-events #31350
Conversation
Thanks for the pull request, @navinkarkera! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
8aba443
to
0de012b
Compare
You don't have to write a step-by-step guide of how to test the events with receivers and all, but at least you could describe how to trigger them, so it's easier to test. |
@mariajgrimaldi Sorry, I am not in front of my system now but I'll soon figure out steps to test and post it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@navinkarkera Changes look good to me at first pass. But the CI is failing. But it doesn't seem to connected to the changes in this PR, but instead some dependencies in enterprise. Can you kindly rebase this to master and see if the CI passes?
@mariajgrimaldi What would be right way to test the values these events get, assuming @navinkarkera adds the instructions on how to trigger these events? Do we have something like a |
0de012b
to
8af8dee
Compare
@tecoholic: we added a line that logs the event responses for just triggers when in a production environment, so it's not that useful for dev (I'll make a mental note on that). What a usually do is create a dummy receiver that logs the input data, that could work here. |
@mariajgrimaldi Thank you for the dummy receiver suggestion. I will try to add something like that for testing. @navinkarkera Everything looks good to me. I will test this tomorrow. |
8af8dee
to
a2c935d
Compare
@mariajgrimaldi @tecoholic I have added test instructions in description. While doing so I found that publish signal is only emitted for We want to be notified about the published vertical and video blocks. If this is not possible here, we would need to handle this in consumer. I'll investigate bit more and come back on this. |
This is understandable given the smallest content we can publish from the Studio is a Unit (VerticalBlock). Good luck with your testing. |
a2c935d
to
76a9e31
Compare
Makes sense. We are now handling this in the xblock provider implemented in course-discovery ( ref MR: openedx/course-discovery#3692 ). It fetches all vertical and video blocks under the published item and processes it. |
4b519d4
to
924c820
Compare
924c820
to
123abd5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@navinkarkera 👍 LGTM
- I tested this: Verified that the signals are being sent when changes are made in studio
- I read through the code
- I checked for accessibility issues - NA
- Includes documentation
123abd5
to
c03d55a
Compare
@mariajgrimaldi Gentle reminder. |
@navinkarkera: I'm out until Dec 20, I'll check on this when I'm back. Sorry for the delay. Unless @felipemontoya can pick this up. |
69c18fc
to
63e148c
Compare
Friendly ping on this @mariajgrimaldi :) |
99d012d
to
cd28c4c
Compare
Hello! I was out on vacation. I'll be checking these comments right away :) |
hello @mphilbrick211: should someone from Enterprise Markhors approve this PR also? According to this comment, they're also involved in this work, and we'll probably need their support for merging. |
@navinkarkera: exciting work! Thanks for all the info context. I'm happy with the state of the PR and all the discussion surrounding it. Do you know if there is anything else that should be addressed? |
I reran the tests, but they kept failing. It seems related to the runner / system itself 🤔. Update: it seems to be working now :) |
@mariajgrimaldi Sure, I'll let my team know and will also take a look myself. |
@mariajgrimaldi Thanks! Nothing to add from my side. |
cd28c4c
to
4f59faf
Compare
@mariajgrimaldi Updated the MR. It should be ready for merge, let me know if something is missing. |
`openedx-events`: `XBLOCK_DELETED`, `XBLOCK_DUPLICATED` and `XBLOCK_PUBLISHED` are hooked into appropriate functions to publish xblock changes.
4f59faf
to
df9f88c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll contact the team in charge, so they merge when convenient. Thank you again for your work!
@navinkarkera 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
EdX Release Notice: This PR has been deployed to the production environment. |
Hi! Would it be possible to put these events behind a feature flag for now? edx.org isn't set up for them, and we're seeing some errors because of it. |
@dianakhuang: hey there! thanks for the report. @sameenfatima78 @navinkarkera what do you say? We could do something like:
Let me know if you need help with this! I'd be glad to help. |
@mariajgrimaldi Thanks, created #31813. Please review and merge. |
Description
openedx-events
:XBLOCK_DELETED
,XBLOCK_DUPLICATED
andXBLOCK_PUBLISHED
are hooked into appropriate functions to publish xblock changes.These signals are implemented in openedx/openedx-events#143 to handle changes to xblocks in taxonomy-connector/course-discovery.
Supporting information
Testing instructions
cms/djangoapps/contentstore/signals/handlers.py
make lms-up studio-up studio-logs
Deadline
"None" if there's no rush, or provide a specific date or event (and reason) if there is one.
Pre-merge checklist: